-
Notifications
You must be signed in to change notification settings - Fork 325
oracle-visualvm-issues-623 Added JAVA_HOME to visulavm luncher & conf #643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Dasarathi Rout <[email protected]>
visualvm/launcher/visualvm
Outdated
@@ -63,7 +63,11 @@ esac | |||
|
|||
|
|||
if [ -f "$progdir"/../lib/visualvm/etc/visualvm.conf ] ; then | |||
visualvm_jdkhome="$basedir" | |||
if [[ -d "$JAVA_HOME" && -f "$JAVA_HOME/bin/java" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Windows version also takes JDK_HOME into consideration, maybe it would be better to have the same behaviour.
Signed-off-by: Dasarathi Rout <[email protected]>
Signed-off-by: Dasarathi Rout <[email protected]>
Signed-off-by: Dasarathi Rout <[email protected]>
Signed-off-by: Dasarathi Rout <[email protected]>
visualvm/launcher/visualvm
Outdated
# Check if JAVA_HOME is set and valid | ||
elif [ -n "${JAVA_HOME}" ] && [ -d "${JAVA_HOME}" ] && [ -f "${JAVA_HOME}/bin/java" ]; then | ||
visualvm_jdkhome="${JAVA_HOME}" | ||
fi | ||
|
||
if [ -f "$progdir"/../lib/visualvm/etc/visualvm.conf ] ; then | ||
visualvm_jdkhome="$basedir" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this overwrite the visualvm_jdkhome if the config file is present, even if visualvm_jdkhome is not defined in it ?
We only want to override JAVA_HOME if visualvm.conf is found AND visualvm_jdkhome is defined in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate any insights or corrections you might have.
Won't this overwrite the visualvm_jdkhome if the config file is present, even if visualvm_jdkhome is not defined in it ?
Setting default value for luncher visualvm_jdkhome
based on JDK_HOME
or JAVA_HOME
env.
after we check for visualvm_jdkhome in visualvm.conf (it might not be defined here ) and overide with privious JDK_HOME
or JAVA_HOME
env any.
We only want to override JAVA_HOME if visualvm.conf is found AND visualvm_jdkhome is defined in it.
My understanding bellow order
- we default to env var
- then visualvm.conf configuration (if defined , else env )
- --jdkhome argument override env & conf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your second example covers the case when the config file is in etc/
But line 66/73 overwrites the variable you set in the patch if the config file is found in lib/
if [ -f "$progdir"/../lib/visualvm/etc/visualvm.conf ] ; then visualvm_jdkhome="$basedir"
Signed-off-by: Dasarathi Rout <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM.
Note that I haven't tested this, only eyeballed.
visualvm/launcher/visualvm
Outdated
fi | ||
} | ||
|
||
set_jdk_java_home_from_conf(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: set_jdk_java_home
would be a better name, as this falls back to environment variables.
Signed-off-by: Dasarathi Rout <[email protected]>
oracle-visualvm-issues-#623 Added JAVA_HOME to visulavm luncher & conf